Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Release Builds with Trimming enabled crashing on launch with NLog Integration #3743

Merged
merged 9 commits into from
Nov 20, 2024
Merged

fix: Release Builds with Trimming enabled crashing on launch with NLog Integration #3743

merged 9 commits into from
Nov 20, 2024

Conversation

bricefriha
Copy link
Contributor

@bricefriha bricefriha commented Nov 11, 2024 •
edited
Loading

This PR aims to fix #3680, an issue that causes a crash when using NLog with FailedRequestStatusCodes options in a MAUI app with Trimming enabled in Release.

Note

we needed to update Nlog to 5.2 to be able to use RegisterType for this fix

worth mentioning: #3698

@bricefriha
Copy link
Contributor Author

@jamescrosswell About #3698, do you think my PR would be problematic for other platforms?
I mean, in a way that would make it safer to simplify SentryNLogOptions's complexity first.

Or can we go ahead with this change for now?

@jamescrosswell
Copy link
Collaborator

@jamescrosswell About #3698, do you think my PR would be problematic for other platforms? I mean, in a way that would make it safer to simplify SentryNLogOptions's complexity first.

Or can we go ahead with this change for now?

Not sure I follow. This PR is to address #3698 right? I can't see anything in the PR (yet) that is platform specific.

Have you had a chance to test this out yet, to make sure it resolves the issue? It's another one that's hard to test in unit tests due to the need to enable trimming.

@bricefriha
Copy link
Contributor Author

@jamescrosswell Sorry, about that.

The PR is for #3680, but you mentioned in #3698 that we needed to bump the NLog and I did this here as part of the fix.
#3698 is about changing the complexity of the options, so I was just wondering if you think it would be better to wait for this to be trackle before merging the PR?

Have you had a chance to test this out yet, to make sure it resolves the issue?

Yes it's all good, I'm just waiting for your input to mark this PR as reviewable

@jamescrosswell
Copy link
Collaborator

Yes it's all good, I'm just waiting for your input to mark this PR as reviewable

If you're waiting for input on this PR, you should mark it ready for review... or is there another discussion I'm neglecting somewhere?

@bricefriha bricefriha marked this pull request as ready for review November 12, 2024 05:06
@jamescrosswell
Copy link
Collaborator

#3698 is about changing the complexity of the options, so I was just wondering if you think it would be better to wait for this to be trackle before merging the PR?

Ah I see, no I don't think we need to change the way the options are structured if this PR resolves the original issue. Changing the options would be quite complex - definitely couldn't be done in a point release (would have to happen in a major version bump) so best to keep separate from this PR.

@bricefriha
Copy link
Contributor Author

so best to keep separate from this PR.

Sounds good. Well, there you have it! 😁

@jamescrosswell jamescrosswell changed the title fix: prevent Release Builds with Trimming ON Crashing on Launch when using NLog Integration and FailedRequestStatusCodes Options fix: Release Builds with Trimming enabled crashing on launch with NLog Integration Nov 16, 2024
@jamescrosswell jamescrosswell mentioned this pull request Nov 16, 2024
Closed
jamescrosswell
jamescrosswell approved these changes Nov 20, 2024
View reviewed changes
Copy link
Collaborator

@jamescrosswell jamescrosswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks @bricefriha !

@jamescrosswell jamescrosswell self-requested a review November 20, 2024 08:09
@jamescrosswell jamescrosswell merged commit d93f96b into main Nov 20, 2024
22 checks passed
@jamescrosswell jamescrosswell deleted the fix/#3680_Nlog branch November 20, 2024 09:26
bruno-garcia
bruno-garcia reviewed Nov 22, 2024
View reviewed changes
CHANGELOG.md
@@ -26,7 +26,8 @@

### Fixes

- Fixed ArgumentNullException in FormRequestPayloadExtractor when handling invalid form data on ASP.NET ([#3734](https://github.com/getsentry/sentry-dotnet/pull/3734))
- ArgumentNullException in FormRequestPayloadExtractor when handling invalid form data on ASP.NET ([#3734](https://github.com/getsentry/sentry-dotnet/pull/3734))
- Crash when using NLog with FailedRequestStatusCodes options in a Maui app with Trimming enabled ([#3743](https://github.com/getsentry/sentry-dotnet/pull/3743))
Copy link
Member

@bruno-garcia bruno-garcia Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAUI is all caps. Maui is a city in Hawaii

@github-actions GitHub Actions
Copy link
Contributor

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- `Release` Builds with Trimming enabled crashing on launch with NLog Integration ([#3743](https://github.com/getsentry/sentry-dotnet/pull/3743))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 84d00f7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruno-garcia bruno-garcia bruno-garcia left review comments

@bitsandfoxes bitsandfoxes Awaiting requested review from bitsandfoxes

@jamescrosswell jamescrosswell Awaiting requested review from jamescrosswell jamescrosswell is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Release Builds Crashing on Launch when using NLog Integration and FailedRequestStatusCodes Options
3 participants
@bricefriha @jamescrosswell @bruno-garcia